Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mixing: Add mixpool package. #3082

Merged
merged 1 commit into from
May 15, 2024
Merged

mixing: Add mixpool package. #3082

merged 1 commit into from
May 15, 2024

Conversation

jrick
Copy link
Member

@jrick jrick commented Apr 5, 2023

The mixpool package implements a memory pool of recently observed mix messages. Similar to the transaction mempool, the mixpool allows these messages to be temporarily stored in memory to be relayed through the P2P network. It handles message acceptance, expiry, UTXO ownership proof checks, and that previously referenced messages have also been accepted to the mixpool.

The mixpool is designed with both full-node and wallet usage in mind, providing all of these same acceptance rules to mixing wallets with the exception of UTXO proof checks. For wallets, it also implements query functions for messages matching compatible pairings and messages belong to ongoing sessions.

Requires #3207.

@jrick jrick force-pushed the mixpool branch 4 times, most recently from 2d0d3c6 to 36b835e Compare April 5, 2023 15:48
@davecgh davecgh added this to the 1.9.0 milestone May 23, 2023
@jrick jrick changed the title mixing: introduce module mixing: add mixpool package Nov 7, 2023
@davecgh davecgh changed the title mixing: add mixpool package mixing: Add mixpool package. Nov 20, 2023
@jrick jrick force-pushed the mixpool branch 3 times, most recently from 5c698ec to 18f2b89 Compare March 20, 2024 13:50
@jrick jrick force-pushed the mixpool branch 4 times, most recently from cfa0c3d to c19fc08 Compare May 11, 2024 04:06
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think almost all of this should be an internal package, just like mempool. Since mixing is a development module for the time being, it's fine, but ultimately I really don't want to go back to having a huge surface of exposed code that really is specifically for the server itself.

I expect that some of it the code should be shared, akin to how blockchain/standalone does it, but pretty much everything in mixpool.go, mixpool_test.go, and log.go, at least, should be internal.

@jrick
Copy link
Member Author

jrick commented May 13, 2024

I think almost all of this should be an internal package, just like mempool. Since mixing is a development module for the time being, it's fine, but ultimately I really don't want to go back to having a huge surface of exposed code that really is specifically for the server itself.

I expect that some of it the code should be shared, akin to how blockchain/standalone does it, but pretty much everything in mixpool.go, mixpool_test.go, and log.go, at least, should be internal.

Not possible with current design.

$ pwd                  
/home/jrick/src/dcrwallet
$ find . -name '*.go' | xargs grep mixpool.Pool
./wallet/mixing.go:func (w *mixingWallet) Mixpool() *mixpool.Pool {
./wallet/wallet.go:	mixpool   *mixpool.Pool

This is because the mixclient package expects the wallet interface to provide an accessor function for the mixpool.

@jrick jrick force-pushed the mixpool branch 5 times, most recently from 39e3711 to f056620 Compare May 14, 2024 14:07
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More to review, but publishing it thus far so you can address it meanwhile.

var (
// ErrChangeDust is returned by AcceptMessage if a pair request's
// change amount is dust.
ErrChangeDust = errors.New("change output is dust")
Copy link
Member

@davecgh davecgh May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also ignoring all of the best practices in the repo for error handling as mentioned in the PR this one depends on.

However, it's fine for the development module, but it's something we'll want to address.

Copy link
Member Author

@jrick jrick May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't return these bare anyways, they are always wrapped as a RuleError, but are defined as variables so callers can test for the precise reason with errors.Is. This isn't that much unlike the error code method, but reads nicer.

mixing/mixpool/log.go Outdated Show resolved Hide resolved
latestKE: make(map[idPubKey]*wire.MsgMixKeyExchange),
sessions: make(map[[32]byte]*session),
sessionsByTxHash: make(map[chainhash.Hash]*session),
epoch: 10 * time.Minute, // XXX: mainnet epoch: add to chainparams
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something we need to merge this, but it would probably be nicer to just take the epoch as a part of configuration for the pool rather than having to worry about access to chain params, and needing an exported method to return it.

mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Show resolved Hide resolved
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 review. More coming.

mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
if !mixing.VerifySignedMessage(msg) {
return nil, ruleError(ErrInvalidSignature)
}
id := (*idPubKey)(msg.Pub())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More panic footgunnage.

mixing/mixpool/mixpool.go Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
@jrick
Copy link
Member Author

jrick commented May 15, 2024

sorry, messed up my history and ended up pushing mixclient onto here. squashed all the wip commits down and force pushed just mixpool again.

mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool.go Outdated Show resolved Hide resolved
mixing/mixpool/mixpool_test.go Show resolved Hide resolved
mixing/utxoproof/utxoproof.go Outdated Show resolved Hide resolved
mixing/utxoproof/utxoproof.go Show resolved Hide resolved
mixing/utxoproof/utxoproof.go Show resolved Hide resolved
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this looks good to go now. Please squash and rebase it, and I'll get it merged. Nice job overall!

The mixpool package implements a memory pool of recently observed mix
messages. Similar to the transaction mempool, the mixpool allows these
messages to be temporarily stored in memory to be relayed through the P2P
network. It handles message acceptance, expiry, UTXO ownership proof checks,
and that previously referenced messages have also been accepted to the
mixpool.

The mixpool is designed with both full-node and wallet usage in mind,
providing all of these same acceptance rules to mixing wallets with the
exception of UTXO proof checks. For wallets, it also implements query
functions for messages matching compatible pairings and messages belong to
ongoing sessions.
@davecgh davecgh merged commit 9338fc8 into decred:master May 15, 2024
2 checks passed
@jrick jrick deleted the mixpool branch May 15, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants